If these tags are okay, I can add functions with precomputed states if desired.

I took this for a commit from @jonasnick, but I am actually unsure what the purpose is of the magic. Absent programming errors inside the library, could something go wrong if there was no magic field? Are there guidelines of when to use a magic (most structs don't contain any)?

I think the magics are a relative recent addition, and just there for assurance, so that incorrect use can be detected. In correct usage, they serve no function.

I think the magics are a relative recent addition, and just there for assurance, so that incorrect use can be detected. In correct usage, they serve no function.

Adding fields to this struct is, I think, an incompatible API break. If a user of the library uses an old version of the header, and links against a new version of the library, I believe the library will access uninitialized fields. Is that right?

In theory, that's ok, because we're still in 0.x releases, so any breaks are allowed by SemVer, but this seems unnecessarily burdensome for users. Ideally, extra fields to the struct only affect calls that are impossible to make in old code (e.g. only by new API functions, or when function arguments are set to constants that don't exist in old versions).

Idly wondering: can we (ab)use the magic for this? Change the magic in the header, and make the library check whether the old or the new magic is used, and trigger behavior based on that.

I believe the library will access uninitialized fields. Is that right?

That is right, good catch.

There could be multiple solutions:

1. As you suggest, the magic can be used as a version field.
2. The s2c_opening struct itself contains a magic field. Instead of calling secp256k1_schnorrsig_s2c_opening_init(s2c_opening) to initialize it inside the signing function, we could have the caller do this instead and check that the magic is set properly.
3. not adding these fields to extraparams but add another function secp256k1_schneorrsig_sign_s2c instead.

It seems to me that option 2 is worse, as if the opening magic is not set properly (e.g. by a user using the old headers with the new library), then the function would error. This is better than proceeding silently, but it still is incompatible and forces the user act. The alternative to erroring is to ignore these fields to maintain compatibility, but that would be strange for new library users that want to use the functionality and don't see a proper error message when they fail to initialize the opening struct.

Option 3 seems backwards as the point of secp256k1_schnorrsig_extraparams struct was, I presume, to be able to extend it in a backwards compatible fashion.

Option 2 probably makes sense to do for the same reason that the extraparams struct is initialized by the user - to prevent corrupting memory in case the user passes the wrong pointer, and to possibly use it as a version field in the future.

TL;DR: I think defining a new magic in the extraparams struct is the best option, and we should also have the caller initialize the secp256k1_schnorrsig_s2c_opening struct with the magic. What do you think?

As a sidenote: maybe it would be a good to introduce a version check or init function for this library in general, so that if breaking changes are necessary in the future, it could be caught early at runtime.

The extraparams struct was built with the intention that magic acts as versioning for the struct and therefore allows backward-compatible changes (there's some mention of this in the original schnorrsig_sign_custom PR). I implemented a variant of the idea here. I was imagining to change schnorrsig_sign_custom to something like this:

int secp256k1_schnorrsig_sign_custom(..., secp256k1_schnorrsig_extraparams *params_in) {
    secp256k1_schnorrsig_extraparams params;

    if (memcmp(params_in->magic, old_magic, sizeof(params_in->magic)) == 0) {
        secp256k1_extraparams_old *params_tmp;
        copy_old_into_new(&params, params_tmp);
    } else if (memcmp(params_in->magic, SECP256K1_SCHNORRSIG_EXTRAPARAMS_MAGIC, sizeof(params_in->magic)) == 0) {
        params = *params_in;
    }
    /* ... */
}

While this does work in practice, I'm not sure how compliant this is with the C standard. It would access the old extraparams struct through a pointer to a new extraparams struct. Padding shouldn't be an issue here because it only accesses the first member of the struct.

It would be better if there was a solution where correctness is more obvious.

I'm not sure how compliant this is with the C standard.

If you read the standard strictly, then it's just UB to have conflicting declarations for the same function: https://stackoverflow.com/a/69620952

In practice, and probably even with a more pragmatic reading of the standard, it's unclear what should go wrong. Following your example code, the callee will read the magic value by accessing bytes through an lvalue of type unsigned char. That's always allowed (even if the new struct type has stricter alignment requirements than the old struct type). Moreover, the magic is guaranteed to be at the beginning of the struct, and it's explicitly allowed to use pointers to a struct as pointers to the first element (after an appropriate cast). After determining the version, the callee will read the struct members through an lvalue of the correct type (not identical type with same name but "compatible type"), which is also allowed.

As the callee starts with reading characters, it's very much like passing an unsigned char* directly to the function and then casting it a pointer to the right struct type. I think we can do this in practice.

Thanks for the helpful inputs everyone!

I added a new commit use the magic in the schnorrsig extraparams struct for versioning that implements this for easier review. I can squash it in the end. Please take a look.

Would it be possible to add a succinct description of the actual protocol to this header, or a reference to it?

The idea is that this is essentially defining a new cryptographic protocol that isn't formally specified anywhere. If so, it'd be good to have the specification right here, so someone doesn't need to go read through the implementation in order to figure out what the exact protocol is.

That makes it easier to separately review the protocol itself, and compare the implementation to that protocol.

As an example, see https://github.com/bitcoin-core/secp256k1/pull/1129/files#diff-d3089adfea8d8bbc31e130bb67389ab9ef45ea4abf4a79eceec63a037359f39dR10-R44 for example, which does so for ElligatorSwift.

I added a specification in the header (in a new commit for easier review, will squash in the end).

The spec is mostly a copy/paste from

• https://github.com/BlockstreamResearch/secp256k1-zkp/blob/6152622613fdf1c5af6f31f74c427c4e9ee120ce/include/secp256k1_ecdsa_s2c.h#L100

A very similar spec was present in @jonasnick's original PR:

• https://github.com/bitcoin-core/secp256k1/pull/590/files#diff-75af67a78d848e432dcf1b975d64978c8d731530fcbde6c9b15edf7da556af00R65

Here as well, I think it'd be good to add a specification of the Anti-Exfil protocol, and/or references to such a specification.